Add --overlay and --ro-overlay command line options #167
Conversation
These enable bubblewrap to create overlay mounts. This will be useful for an ostree-based build system we use where overlayfs ensures that none of the ostree hard-linked files I checkout get modified. Currently we use a maze of bash/unshare/mount/sudo/chroot where bubblewrap will be much nicer. This commit contains a bit of string manipulation, which isn't particularly fun to write in C. Hopefully I got it right. No tests are written to exercise this new feature.
|
Can one of the admins verify this patch?
|
|
bot, add author to whitelist |
|
I'm assuming you looked at the rofiles-fuse approach but chose not to? So...one thing to keep in mind is that currently, this patch will only work if bwrap is setuid - as far as I know, the unprivileged overlayfs (and other fs) discussion is still outstanding. |
|
Also, this exposes the multi-layer version of overlayfs, which is only supported since Linux 4.0, which is pretty recent, so it may not be a great thing to depend on if you want to e.g. run on current LTS kernels. |
Yes. I need unshare, chroot, mount, etc. anyway so some sort of setuid is needed. I figure I may as well use overlayfs as it's a more general solution than rofiles-fuse. I do a bunch of apt-get installs during my build and you can't guarantee that all of the maintainer-scripts, etc. will not attempt to modify files in-place - and so fail. I can imagine future optimisations too where I only check-in the upper layer to ostree and have ostree combine layers, rather than having to walk the entire filesystem tree which can be a little slow even with
I'll document this caveat. |
Yes, but only if you use
I'll document this requirement. |
|
As far as overlayfs vs rofiles-fuse, see this Fedora tracker bug for rpm-ostree, which currently uses rofiles-fuse on top of ostree to implement package layering. I've submitted a few patches. What we haven't done yet is fully unify the core for rpm-ostree so that the treecompose side also runs unprivileged with rofiles, but I don't think there'll be too many things to patch that I care about. |
| strappend(&sb, ":"); | ||
| strappend(&sb, "/oldroot"); | ||
| /* Resolve absolute symlinks before we remount under /oldroot: */ | ||
| strappend(&sb, realpath (token, buf)); |
There was a problem hiding this comment.
Need to handle errors from realpath here.
|
I dunno about this. As long as the kernel people are not comfortable exposing overlayfs to users, I'm not confortable taking on the responsibility for that... |
|
We have made some changes to overlay to check on the mounters credentials, for access. But opening up overlay without kernel guys saying it ok, would be VERY risky. |
|
I understand the reluctance but I'd like to point out that bubblewrap provides a more restricted environment than by default on linux. Specifically due to
So I guess I'll just have to carry this patch myself |
|
@wmanley PR_SET_NO_NEW_PRIVS is required by the kernel to use unprivileged user namespaces too, and still the kernel does not allow such use to mount overlayfs instances. This is the reason we're worrying. |
|
Works fine for me, also when executed as a non-root user with a non-setuid bubblewrap binary. |
|
Time to revisit now Linux 5.11 supports it? (And Ubuntu has supported it for a long time through a kernel patch...) |
|
Currently a happy user of this patch, also with a non-root user and non-setuid binary. One note: the overlayfs docs specify that multiple lower layers are given in top-to-bottom order, which means that using this patch, Maintainers, has the context around overlayfs changed enough in the last five years for you to be more comfortable with this feature? If so, I'd be happy to open a new PR with the above change and make any needed further improvements. |
I'd consider a PR that enabled this on kernels where overlayfs is allowed for non-root users, and only when bubblewrap is not setuid (same restriction as On kernels where overlayfs is not allowed for non-root users, bubblewrap should not allow it either. Similarly, when bubblewrap is setuid root, we should not allow this: with a setuid bubblewrap (as used on Debian <= 10, etc.), there's too high a risk of bubblewrap allowing something that the kernel considers unsafe. |
| die ("Out of memory"); | ||
| } | ||
|
|
||
| strcpy(dest->str + dest->offset, src); |
There was a problem hiding this comment.
I am not comfortable with strcpy being used in a codebase that is sometimes setuid root. However careful you've been to allocate enough memory, and to avoid entering this code while privileged, it's just too easy to get wrong. As a minimum, this should use strncpy - but if it can be made to use things like asprintf then that would likely be safer.
|
|
||
| strappend(&sb, "lowerdir="); | ||
|
|
||
| token = strtok(layers_mut, ":"); |
There was a problem hiding this comment.
I'm not really happy about the amount of strtok() here: C string manipulation is notoriously easy to get wrong in security-sensitive ways, and bubblewrap is sometimes setuid root.
I'd prefer it if the layers can be defined in a way that doesn't need to be parsed, perhaps something like
--overlay-upper /foo/upper --overlay-lower /lower1 --overlay-lower /lower2 [--overlay-rw /foo/work] --overlay-onto /foo/merged
with these options only allowed in exactly that order (similar to how the --perms and --size modifiers work).
| in all the <arg choice="plain">LAYERS</arg>. The paths listed in | ||
| LAYERS may not contain a comma (,) or a colon (:). |
There was a problem hiding this comment.
If there are syntactic restrictions like this, the code should enforce them.
| argv += 2; | ||
| argc -= 2; | ||
| } | ||
| else if (strcmp (arg, "--overlay") == 0) |
There was a problem hiding this comment.
This option should be rejected when privileged, to avoid handing out abilities that the kernel thinks should only be given to root.
| argv += 3; | ||
| argc -= 3; | ||
| } | ||
| else if (strcmp (arg, "--ro-overlay") == 0) |
#547 is a continuation of this. |
These enable bubblewrap to create overlay mounts. This will be useful for
an ostree-based build system we use where overlayfs ensures that none
of the ostree hard-linked files I checkout get modified. Currently we
use a maze of bash/unshare/mount/sudo/chroot where bubblewrap will be much
nicer.
This commit contains a bit of string manipulation, which isn't
particularly fun to write in C. Hopefully I got it right.
No tests are yet written to exercise this new feature.
Thinking about it I should probably validate that the paths contain no commas to prevent a user from passing arbitrary options to the overlayfs kernel module.